Update Enumerable.Min/Max for all IBinaryIntegers #96605
Conversation
The implementations are special-casing most of the built-in ones, in order to delegate to the IBinaryInteger-constrained helper, but it was missing Int128, UInt128, and char. ?hese won't be vectorized, but they'll at least use the more streamlined non-vectorized implementations.
|
Tagging subscribers to this area: @dotnet/area-system-linq Issue DetailsThe implementations are special-casing most of the built-in ones, in order to delegate to the IBinaryInteger-constrained helper, but it was missing Int128, UInt128, and char. ?hese won't be vectorized, but they'll at least use the more streamlined non-vectorized implementations.
|
ghost
assigned 
There was a problem hiding this comment.
If vectorization isn't available for the new types, what is the primary motivation for making this change? Is it that IBinaryInteger comparison is generally faster than using Comparer<TDefault>? Or is it simply the fact that we extract the span and avoid allocating an enumerator? If it's the latter case, couldn't this be extracted to the outer method so that it can be applied to all types?
|
Previously a) it was a bit faster and b) IsSupported could ironically only be called with a supported type or else it would throw. But the former was improved in the JIT and the latter was fixed late in 8, so we should be able to make it work with an IComparable-based implementation. The primary benefit is being able to work on the span and avoid the enumerator costs. |
|
Although https://github.com/dotnet/runtime/pull/96570/files#diff-a7055cca652c7bf1b5af1f4f86111a7949edc40480117bf2fd7651b9338458ca needs to be merged first to remove the constraint from TryGetSpan. |
|
Since this is a small change, I suggest we merge this, and then we can subsequently look at improving Min/Max to use a span for other types. I have another branch where I'm doing so for more operators and I can incorporate it into that. |
I gave this a go, and the reliance on |
PR dotnet#96605 added new generic arguments to the unit tests which need to be preserved in the rd.xml since the test suite is not yet trim-compatible.
PR #96605 added new generic arguments to the unit tests which need to be preserved in the rd.xml since the test suite is not yet trim-compatible.
PR dotnet#96605 added new generic arguments to the unit tests which need to be preserved in the rd.xml since the test suite is not yet trim-compatible.
The implementations are special-casing most of the built-in ones, in order to delegate to the IBinaryInteger-constrained helper, but it was missing Int128, UInt128, and char. ?hese won't be vectorized, but they'll at least use the more streamlined non-vectorized implementations.